feat: support multiple job agent configs for a deployment#807
Merged
adityachoudhari26 merged 14 commits intomainfrom Feb 19, 2026
Merged
feat: support multiple job agent configs for a deployment#807adityachoudhari26 merged 14 commits intomainfrom
adityachoudhari26 merged 14 commits intomainfrom
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a DeploymentJobAgent type and Deployment.jobAgents; implements DeploymentAgentsSelector (CEL-based) to pick agents; threads JobAgent through job creation; ExecuteRelease and event handlers create/persist/dispatch one job per selected agent; DB schema, mappers, tests, and migrations updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Exec as "Executor\n(ExecuteRelease)"
participant Selector as "DeploymentAgentsSelector"
participant Store as "Store/DB"
participant Factory as "Job Factory\n(CreateJobForRelease)"
participant Dispatcher as "Dispatcher"
Exec->>Selector: SelectAgents(deployment, release)
Selector->>Store: fetch env/resource and JobAgent refs
Store-->>Selector: JobAgent objects / context
Selector-->>Exec: []*JobAgent (selected)
loop per JobAgent
Exec->>Factory: CreateJobForRelease(ctx, release, jobAgent, ...)
Factory->>Store: upsert job
Store-->>Factory: persisted job
Factory-->>Exec: job
Exec->>Dispatcher: dispatch(job)
Dispatcher-->>Exec: dispatch result/status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go`:
- Line 298: Remove the redundant inline comment "// Upsert the new jobs" that
simply restates the following loop's action; locate the comment in deployment.go
near the loop that performs job upserts and delete it so the code remains
self-explanatory without restating behavior.
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Around line 242-244: The doc comment for CreateJobsForRelease is inconsistent:
update the top comment to describe that it creates multiple jobs for a given
release (reflecting multi-job semantics) and mentions merged settings from
JobAgent + Deployment remain; adjust wording from singular ("a job") to plural
("jobs") and ensure it still notes that the function is pure (no writes) and
returns ([]*oapi.Job, error).
- Around line 302-323: The trace metadata currently uses a single generated
jobId variable but each job gets its real ID inside buildJobForAgent, so update
the logic to record the actual job IDs: for each jobAgent call buildJobForAgent
(function buildJobForAgent) first, obtain the returned job.Id, then call
action.AddStep/AddMetadata using that job.Id (and job.Status/release info)
instead of the shared jobId; this ensures the jobs slice append and the trace
metadata reference the real per-job identifier rather than the pre-generated
jobId.
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Around line 85-87: The loop over newJobs currently ignores errors from
e.store.Jobs.Upsert; change it to capture the returned error for each Upsert
call and handle it (e.g., log the error with context including the job ID/name
and return the error to abort execution or collect and return a combined error)
so failures don't silently drop jobs; update the loop around newJobs and
e.store.Jobs.Upsert to check err and propagate or surface it from the enclosing
function (or retry if your retry policy exists).
apps/workspace-engine/pkg/events/handler/deployment/deployment.go
Outdated
Show resolved
Hide resolved
Comment on lines
242
to
244
| // CreateJobsForRelease creates a job for a given release (PURE FUNCTION, NO WRITES). | ||
| // The job is configured with merged settings from JobAgent + Deployment. | ||
| func (f *Factory) CreateJobForRelease(ctx context.Context, release *oapi.Release, action *trace.Action) (*oapi.Job, error) { | ||
| func (f *Factory) CreateJobsForRelease(ctx context.Context, release *oapi.Release, action *trace.Action) ([]*oapi.Job, error) { |
Contributor
There was a problem hiding this comment.
Doc comment is now singular but the method is plural.
This should reflect the new multi-job semantics.
✏️ Proposed doc update
-// CreateJobsForRelease creates a job for a given release (PURE FUNCTION, NO WRITES).
-// The job is configured with merged settings from JobAgent + Deployment.
+// CreateJobsForRelease creates jobs for a given release (PURE FUNCTION, NO WRITES).
+// Jobs are configured with merged settings from JobAgent + Deployment.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // CreateJobsForRelease creates a job for a given release (PURE FUNCTION, NO WRITES). | |
| // The job is configured with merged settings from JobAgent + Deployment. | |
| func (f *Factory) CreateJobForRelease(ctx context.Context, release *oapi.Release, action *trace.Action) (*oapi.Job, error) { | |
| func (f *Factory) CreateJobsForRelease(ctx context.Context, release *oapi.Release, action *trace.Action) ([]*oapi.Job, error) { | |
| // CreateJobsForRelease creates jobs for a given release (PURE FUNCTION, NO WRITES). | |
| // Jobs are configured with merged settings from JobAgent + Deployment. | |
| func (f *Factory) CreateJobsForRelease(ctx context.Context, release *oapi.Release, action *trace.Action) ([]*oapi.Job, error) { |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go` around lines 242 - 244,
The doc comment for CreateJobsForRelease is inconsistent: update the top comment
to describe that it creates multiple jobs for a given release (reflecting
multi-job semantics) and mentions merged settings from JobAgent + Deployment
remain; adjust wording from singular ("a job") to plural ("jobs") and ensure it
still notes that the function is pure (no writes) and returns ([]*oapi.Job,
error).
Comment on lines
302
to
323
| jobId := uuid.New().String() | ||
|
|
||
| if action != nil { | ||
| action.AddStep("Create job", trace.StepResultPass, | ||
| fmt.Sprintf("Job created successfully with ID %s for release %s", jobId, release.ID())). | ||
| AddMetadata("job_id", jobId). | ||
| AddMetadata("job_status", string(oapi.JobStatusPending)). | ||
| AddMetadata("job_agent_id", *jobAgentId). | ||
| AddMetadata("release_id", release.ID()). | ||
| AddMetadata("version_tag", release.Version.Tag) | ||
| } | ||
|
|
||
| mergedConfig, err := f.buildJobAgentConfig(release, deployment, jobAgent) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get merged job agent config: %w", err) | ||
| } | ||
|
|
||
| dispatchContext, err := f.buildDispatchContext(release, deployment, jobAgent, mergedConfig) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build dispatch context: %w", err) | ||
| } | ||
|
|
||
| return &oapi.Job{ | ||
| Id: jobId, | ||
| ReleaseId: release.ID(), | ||
| JobAgentId: *jobAgentId, | ||
| JobAgentConfig: mergedConfig, | ||
| Status: oapi.JobStatusPending, | ||
| CreatedAt: time.Now(), | ||
| UpdatedAt: time.Now(), | ||
| Metadata: make(map[string]string), | ||
| DispatchContext: dispatchContext, | ||
| }, nil | ||
| for _, jobAgent := range jobAgents { | ||
| action.AddStep("Create job", trace.StepResultPass, | ||
| fmt.Sprintf("Job created successfully with ID %s for release %s", jobId, release.ID())). | ||
| AddMetadata("job_id", jobId). | ||
| AddMetadata("job_status", string(oapi.JobStatusPending)). | ||
| AddMetadata("job_agent_id", jobAgent.Id). | ||
| AddMetadata("release_id", release.ID()). | ||
| AddMetadata("version_tag", release.Version.Tag) | ||
| } | ||
| } | ||
|
|
||
| jobs := make([]*oapi.Job, 0) | ||
| for _, jobAgent := range jobAgents { | ||
| job, err := f.buildJobForAgent(release, deployment, jobAgent) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build job for agent: %w", err) | ||
| } | ||
| jobs = append(jobs, job) | ||
| } |
Contributor
There was a problem hiding this comment.
Trace metadata uses a shared jobId that doesn’t match the created jobs.
jobId is generated once but each job gets a different ID in buildJobForAgent, so trace metadata is misleading. Consider recording the actual job.Id after creation.
🧭 Proposed fix to use actual job IDs in trace metadata
- jobId := uuid.New().String()
-
- if action != nil {
- for _, jobAgent := range jobAgents {
- action.AddStep("Create job", trace.StepResultPass,
- fmt.Sprintf("Job created successfully with ID %s for release %s", jobId, release.ID())).
- AddMetadata("job_id", jobId).
- AddMetadata("job_status", string(oapi.JobStatusPending)).
- AddMetadata("job_agent_id", jobAgent.Id).
- AddMetadata("release_id", release.ID()).
- AddMetadata("version_tag", release.Version.Tag)
- }
- }
-
- jobs := make([]*oapi.Job, 0)
+ jobs := make([]*oapi.Job, 0, len(jobAgents))
for _, jobAgent := range jobAgents {
job, err := f.buildJobForAgent(release, deployment, jobAgent)
if err != nil {
return nil, fmt.Errorf("failed to build job for agent: %w", err)
}
+ if action != nil {
+ action.AddStep("Create job", trace.StepResultPass,
+ fmt.Sprintf("Job created successfully with ID %s for release %s", job.Id, release.ID())).
+ AddMetadata("job_id", job.Id).
+ AddMetadata("job_status", string(oapi.JobStatusPending)).
+ AddMetadata("job_agent_id", jobAgent.Id).
+ AddMetadata("release_id", release.ID()).
+ AddMetadata("version_tag", release.Version.Tag)
+ }
jobs = append(jobs, job)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| jobId := uuid.New().String() | |
| if action != nil { | |
| action.AddStep("Create job", trace.StepResultPass, | |
| fmt.Sprintf("Job created successfully with ID %s for release %s", jobId, release.ID())). | |
| AddMetadata("job_id", jobId). | |
| AddMetadata("job_status", string(oapi.JobStatusPending)). | |
| AddMetadata("job_agent_id", *jobAgentId). | |
| AddMetadata("release_id", release.ID()). | |
| AddMetadata("version_tag", release.Version.Tag) | |
| } | |
| mergedConfig, err := f.buildJobAgentConfig(release, deployment, jobAgent) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get merged job agent config: %w", err) | |
| } | |
| dispatchContext, err := f.buildDispatchContext(release, deployment, jobAgent, mergedConfig) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to build dispatch context: %w", err) | |
| } | |
| return &oapi.Job{ | |
| Id: jobId, | |
| ReleaseId: release.ID(), | |
| JobAgentId: *jobAgentId, | |
| JobAgentConfig: mergedConfig, | |
| Status: oapi.JobStatusPending, | |
| CreatedAt: time.Now(), | |
| UpdatedAt: time.Now(), | |
| Metadata: make(map[string]string), | |
| DispatchContext: dispatchContext, | |
| }, nil | |
| for _, jobAgent := range jobAgents { | |
| action.AddStep("Create job", trace.StepResultPass, | |
| fmt.Sprintf("Job created successfully with ID %s for release %s", jobId, release.ID())). | |
| AddMetadata("job_id", jobId). | |
| AddMetadata("job_status", string(oapi.JobStatusPending)). | |
| AddMetadata("job_agent_id", jobAgent.Id). | |
| AddMetadata("release_id", release.ID()). | |
| AddMetadata("version_tag", release.Version.Tag) | |
| } | |
| } | |
| jobs := make([]*oapi.Job, 0) | |
| for _, jobAgent := range jobAgents { | |
| job, err := f.buildJobForAgent(release, deployment, jobAgent) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to build job for agent: %w", err) | |
| } | |
| jobs = append(jobs, job) | |
| } | |
| jobs := make([]*oapi.Job, 0, len(jobAgents)) | |
| for _, jobAgent := range jobAgents { | |
| job, err := f.buildJobForAgent(release, deployment, jobAgent) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to build job for agent: %w", err) | |
| } | |
| if action != nil { | |
| action.AddStep("Create job", trace.StepResultPass, | |
| fmt.Sprintf("Job created successfully with ID %s for release %s", job.Id, release.ID())). | |
| AddMetadata("job_id", job.Id). | |
| AddMetadata("job_status", string(oapi.JobStatusPending)). | |
| AddMetadata("job_agent_id", jobAgent.Id). | |
| AddMetadata("release_id", release.ID()). | |
| AddMetadata("version_tag", release.Version.Tag) | |
| } | |
| jobs = append(jobs, job) | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go` around lines 302 - 323,
The trace metadata currently uses a single generated jobId variable but each job
gets its real ID inside buildJobForAgent, so update the logic to record the
actual job IDs: for each jobAgent call buildJobForAgent (function
buildJobForAgent) first, obtain the returned job.Id, then call
action.AddStep/AddMetadata using that job.Id (and job.Status/release info)
instead of the shared jobId; this ensures the jobs slice append and the trace
metadata reference the real per-job identifier rather than the pre-generated
jobId.
Comment on lines
85
to
87
| for _, job := range newJobs { | ||
| e.store.Jobs.Upsert(ctx, job) | ||
| } |
Contributor
There was a problem hiding this comment.
Handle job persistence errors in the loop.
Upsert failures would silently drop jobs while execution continues, risking inconsistent state.
🛠️ Proposed fix with error handling
span.AddEvent("Persisting job to store")
for _, job := range newJobs {
- e.store.Jobs.Upsert(ctx, job)
+ if err := e.store.Jobs.Upsert(ctx, job); err != nil {
+ span.RecordError(err)
+ span.SetStatus(codes.Error, "failed to persist job")
+ if createJobAction != nil {
+ createJobAction.AddStep("Persist job", trace.StepResultFail, fmt.Sprintf("Failed: %s", err.Error()))
+ }
+ return nil, err
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, job := range newJobs { | |
| e.store.Jobs.Upsert(ctx, job) | |
| } | |
| for _, job := range newJobs { | |
| if err := e.store.Jobs.Upsert(ctx, job); err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "failed to persist job") | |
| if createJobAction != nil { | |
| createJobAction.AddStep("Persist job", trace.StepResultFail, fmt.Sprintf("Failed: %s", err.Error())) | |
| } | |
| return nil, err | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`
around lines 85 - 87, The loop over newJobs currently ignores errors from
e.store.Jobs.Upsert; change it to capture the returned error for each Upsert
call and handle it (e.g., log the error with context including the job ID/name
and return the error to abort execution or collect and return a combined error)
so failures don't silently drop jobs; update the loop around newJobs and
e.store.Jobs.Upsert to check err and propagate or surface it from the enclosing
function (or retry if your retry policy exists).
…multiple-job-agents
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go (1)
151-186:⚠️ Potential issue | 🟠 MajorThis test contradicts ExecuteRelease’s behavior.
ExecuteReleasecurrently returns aNoAgentConfiguredJobslice when no agents are configured, but the test expects an empty slice. This will fail. Align the test (or the implementation) so they match.✅ If keeping current behavior, update the test like this
- require.Empty(t, jobs) + require.Len(t, jobs, 1) + job := jobs[0] + require.Equal(t, oapi.JobStatusInvalidJobAgent, job.Status) - // Verify release was still persisted - _, exists := testStore.Releases.Get(release.ID()) + // Verify release and job were persisted + _, exists := testStore.Releases.Get(release.ID()) require.True(t, exists) + _, jobExists := testStore.Jobs.Get(job.Id) + require.True(t, jobExists)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go` around lines 151 - 186, The test TestExecuteRelease_NoJobAgentConfigured is asserting an empty jobs result but ExecuteRelease returns a slice containing NoAgentConfiguredJob when no job agents are configured; update the test to call executor.ExecuteRelease(ctx, release, nil) and assert that jobs contains a NoAgentConfiguredJob entry (check type/name or expected fields) instead of expecting an empty slice, while still verifying the release was persisted; reference ExecuteRelease and the NoAgentConfiguredJob type in your assertions so the test matches the current implementation.
🧹 Nitpick comments (2)
apps/workspace-engine/oapi/openapi.json (2)
179-207: Clarify howjobAgentscoexists with legacy single-agent fields.With
jobAgentsadded, the schema still marksjobAgentConfigas required and doesn’t describe precedence if both are present. If the new array is intended to replace or override the legacy fields, consider encoding that viaoneOf(or updatingrequired) plus a short description of precedence to avoid client confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/oapi/openapi.json` around lines 179 - 207, The schema currently requires jobAgentConfig while also adding jobAgents, causing ambiguity; update the OpenAPI schema so jobAgents and the legacy single-agent fields (e.g., jobAgentConfig) are mutually exclusive or have a clear precedence: either replace the required array/field list with a oneOf that defines "legacy" (requires jobAgentConfig) vs "multi-agent" (requires jobAgents) variants, or adjust the required array to remove jobAgentConfig and add a concise description on the parent schema explaining precedence/override behavior between jobAgents and jobAgentConfig; reference the schema properties jobAgents and jobAgentConfig (and the parent object) when making these changes so clients can unambiguously know which to use.
240-258: Consider makingifoptional with a sensible default.If unconditional agents are valid, requiring
"if"forces clients to send"if": "true"every time. Consider defaultingifto"true"and removing it fromrequiredto simplify usage.Suggested schema tweak
"if": { "description": "CEL expression to determine if the job agent should be used", - "type": "string" + "type": "string", + "default": "true" }, ... "required": [ "ref", - "config", - "if" + "config" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/oapi/openapi.json` around lines 240 - 258, The DeploymentJobAgent schema currently requires the "if" property which forces clients to send an explicit expression; make "if" optional by removing "if" from the "required" array of DeploymentJobAgent and add a sensible default to its property definition (set the "if" property's default to the string "true" and keep its type as "string") so the agent is treated as unconditional when not provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go`:
- Around line 288-295: Retrigger logic currently looks up agents by the old
job.JobAgentId and skips when that ID is stale; instead, use the deployment's
current selector/list to find the intended agents and retrigger for each of
those. Replace the single lookup using ws.JobAgents().Get(job.JobAgentId) with a
selection from the release/deployment config (e.g., derive the selector or
jobAgent list from the Release/Deployment object) and iterate those agent IDs,
calling jobFactory.CreateJobForRelease(ctx, release, agent, nil) for each
selected agent; keep the existing error handling but ensure you no longer rely
on job.JobAgentId being present.
In `@apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.go`:
- Around line 11-15: The CEL environment build currently ignores errors when
calling
celutil.NewEnvBuilder().WithMapVariables(...).WithStandardExtensions().BuildCached(...),
storing only jobAgentIfEnv; capture the returned error and do not discard it —
either panic immediately if BuildCached returns an error (so jobAgentIfEnv
cannot be used), or change SelectAgents to accept/return an error and propagate
the BuildCached error through SelectAgents; update references to jobAgentIfEnv
and any calls to Compile to handle the possibility of no env by using the
validated env variable or the propagated error.
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Line 32: Add a Go doc comment for the exported function NoAgentConfiguredJob
describing what the function does, the meaning of its parameters (releaseID,
jobAgentID, deploymentName, action *trace.Action) and what it returns
(*oapi.Job), include any important behavior or side-effects callers must know
(e.g., created job represents a "no agent configured" failure/placeholder), and
place it immediately above the function declaration for NoAgentConfiguredJob so
it follows Go exported-symbol documentation conventions.
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Around line 58-62: When Dispatch fails the job is left as Pending; set the job
status to an error state and persist before returning. In the error branch that
handles e.jobAgentRegistry.Dispatch(ctx, newJob) set newJob.Status =
model.JobStatusInvalidIntegration (or your repo's equivalent), call the
executor's job persistence method (e.g. e.jobStore.Update / e.jobRepo.UpdateJob
/ another existing save/update method on the executor) to persist the status
change, record/log any error from that update (keep
span.RecordError/span.SetStatus for the dispatch error), then return the
original dispatch error; ensure you reference e.jobAgentRegistry.Dispatch and
newJob when making the change so the update happens right before returning.
---
Outside diff comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go`:
- Around line 151-186: The test TestExecuteRelease_NoJobAgentConfigured is
asserting an empty jobs result but ExecuteRelease returns a slice containing
NoAgentConfiguredJob when no job agents are configured; update the test to call
executor.ExecuteRelease(ctx, release, nil) and assert that jobs contains a
NoAgentConfiguredJob entry (check type/name or expected fields) instead of
expecting an empty slice, while still verifying the release was persisted;
reference ExecuteRelease and the NoAgentConfiguredJob type in your assertions so
the test matches the current implementation.
---
Duplicate comments:
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Around line 50-51: The call to e.store.Jobs.Upsert(ctx, newJob) is ignoring
errors and can silently drop jobs; update the surrounding function (where Upsert
is called) to capture the returned error, log it with context (including newJob
ID and intent) and return or propagate the error instead of continuing silently;
ensure you handle the error path consistently (e.g., return fmt.Errorf or wrap
with additional context) so callers know persistence failed.
---
Nitpick comments:
In `@apps/workspace-engine/oapi/openapi.json`:
- Around line 179-207: The schema currently requires jobAgentConfig while also
adding jobAgents, causing ambiguity; update the OpenAPI schema so jobAgents and
the legacy single-agent fields (e.g., jobAgentConfig) are mutually exclusive or
have a clear precedence: either replace the required array/field list with a
oneOf that defines "legacy" (requires jobAgentConfig) vs "multi-agent" (requires
jobAgents) variants, or adjust the required array to remove jobAgentConfig and
add a concise description on the parent schema explaining precedence/override
behavior between jobAgents and jobAgentConfig; reference the schema properties
jobAgents and jobAgentConfig (and the parent object) when making these changes
so clients can unambiguously know which to use.
- Around line 240-258: The DeploymentJobAgent schema currently requires the "if"
property which forces clients to send an explicit expression; make "if" optional
by removing "if" from the "required" array of DeploymentJobAgent and add a
sensible default to its property definition (set the "if" property's default to
the string "true" and keep its type as "string") so the agent is treated as
unconditional when not provided.
apps/workspace-engine/pkg/events/handler/deployment/deployment.go
Outdated
Show resolved
Hide resolved
apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.go
Outdated
Show resolved
Hide resolved
apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/workspace-engine/pkg/workspace/jobs/factory.go (2)
55-76:⚠️ Potential issue | 🟡 MinorAdd a doc comment for the exported
InvalidDeploymentAgentsJob.✏️ Suggested doc comment
+// InvalidDeploymentAgentsJob creates an InvalidJobAgent sentinel job when the +// deployment agent selector returns an error (e.g., a CEL evaluation failure). +// The returned job has an empty JobAgentId; callers must not attempt dispatch. func (f *Factory) InvalidDeploymentAgentsJob(releaseID, deploymentName string, action *trace.Action) *oapi.Job {As per coding guidelines, "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobs/factory.go` around lines 55 - 76, Add a short Go doc comment above the exported function InvalidDeploymentAgentsJob that explains its purpose and behavior: that it constructs and returns an oapi.Job representing an invalid deployment agents error for a given release and deployment (including the message, Status set to oapi.JobStatusInvalidJobAgent, timestamps, and metadata initialization), and mention the parameters (releaseID, deploymentName, action) and their roles; keep the comment concise and follow godoc style (starting with the function name).
173-211:⚠️ Potential issue | 🟡 MinorTrace steps record "Pass" before the work they describe has actually succeeded.
"Configure job"(Line 176) and"Create job"(Line 182) are emitted asStepResultPassbeforebuildJobAgentConfig(Line 191) andbuildDispatchContext(Line 196) are called. If either of those fails the action trace shows success for steps that never completed, making trace replay misleading.Move the action steps to after the calls succeed, or record them with a conditional result.
🛠️ Proposed fix: emit trace steps after work completes
- if action != nil { - configMsg := "Applied default job agent configuration" - action.AddStep("Configure job", trace.StepResultPass, configMsg) - } - - jobId := uuid.New().String() - - if action != nil { - action.AddStep("Create job", trace.StepResultPass, - fmt.Sprintf("Job created successfully with ID %s for release %s", jobId, release.ID())). - AddMetadata("job_id", jobId). - AddMetadata("job_status", string(oapi.JobStatusPending)). - AddMetadata("job_agent_id", jobAgent.Id). - AddMetadata("release_id", release.ID()). - AddMetadata("version_tag", release.Version.Tag) - } - mergedConfig, err := f.buildJobAgentConfig(release, deployment, jobAgent) if err != nil { return nil, fmt.Errorf("failed to get merged job agent config: %w", err) } dispatchContext, err := f.buildDispatchContext(release, deployment, jobAgent, mergedConfig) if err != nil { return nil, fmt.Errorf("failed to build dispatch context: %w", err) } + if action != nil { + action.AddStep("Configure job", trace.StepResultPass, "Applied job agent configuration") + } + + jobId := uuid.New().String() + return &oapi.Job{ Id: jobId, ... }, nil + + // (move "Create job" step here, after the Job struct is constructed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobs/factory.go` around lines 173 - 211, The trace steps "Configure job" and "Create job" are being recorded with trace.StepResultPass before the actual work (buildJobAgentConfig and buildDispatchContext) completes; either move the action.AddStep calls so they run after f.buildJobAgentConfig(...) and f.buildDispatchContext(...) return successfully, or emit them initially with a non-passing state (e.g., pending) and update them to StepResultPass only after those functions succeed; locate the action.AddStep calls, the jobId creation and the return of &oapi.Job{...}, and ensure action.AddStep invocations reflect the real outcome of buildJobAgentConfig and buildDispatchContext (or are moved to after those calls).apps/workspace-engine/pkg/events/handler/deployment/deployment.go (1)
141-156:⚠️ Potential issue | 🟠 Major
reconcileTargetsignores the newJobAgentsfield — multi-agent-only deployments are never reconciled.The guard
deployment.JobAgentId != nil && *deployment.JobAgentId != ""was correct before multi-agent support, butDeploymentnow also has aJobAgents *[]DeploymentJobAgentfield. A deployment that configures agents exclusively throughJobAgents(no legacyJobAgentId) will skip the entire reconcile body, so its release targets are never re-evaluated even whenisJobAgentConfigurationChangedcorrectly detects a change.🛠️ Proposed fix
func reconcileTargets(...) error { - if deployment.JobAgentId != nil && *deployment.JobAgentId != "" { + hasAgentId := deployment.JobAgentId != nil && *deployment.JobAgentId != "" + hasJobAgents := deployment.JobAgents != nil && len(*deployment.JobAgents) > 0 + if hasAgentId || hasJobAgents { for _, rt := range releaseTargets { ws.ReleaseManager().DirtyDesiredRelease(rt) }Note:
HandleDeploymentCreated(Line 81) has the same gap for the creation path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go` around lines 141 - 156, reconcileTargets currently skips reconciling when a Deployment uses the new multi-agent field because it only checks Deployment.JobAgentId; change the guard to run the reconcile body when either JobAgentId is set/non-empty OR JobAgents is non-nil with length > 0. Update the same logic in HandleDeploymentCreated so creation-path deployments using JobAgents are not skipped. Locate reconcileTargets and HandleDeploymentCreated and replace the single-field check (deployment.JobAgentId != nil && *deployment.JobAgentId != "") with a combined condition that checks JobAgentId OR (deployment.JobAgents != nil && len(*deployment.JobAgents) > 0) before calling ReleaseManager.DirtyDesiredRelease, RecomputeState and ReconcileTarget.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/events/handler/deployment/deployment.go (1)
158-167: Redundant nil check — the!okbranch already handles the not-found case.
ws.Deployments().Getreturns(nil, false)when not found; the== nilguard at Lines 163-165 is never reached.🧹 Proposed simplification
func getOldDeployment(ws *workspace.Workspace, deploymentID string) (oapi.Deployment, error) { oldDeployment, ok := ws.Deployments().Get(deploymentID) if !ok { return oapi.Deployment{}, fmt.Errorf("deployment %s not found", deploymentID) } - if oldDeployment == nil { - return oapi.Deployment{}, fmt.Errorf("deployment %s not found", deploymentID) - } return *oldDeployment, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go` around lines 158 - 167, The function getOldDeployment contains a redundant nil check: after calling ws.Deployments().Get(deploymentID) you already handle the not-found case with the ok boolean; remove the if oldDeployment == nil branch and simply return *oldDeployment at the end (keeping the existing error returned when !ok). This targets getOldDeployment and the oldDeployment variable returned by ws.Deployments().Get; delete the unnecessary nil guard and rely on the ok check to signal not-found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go`:
- Around line 169-191: The function isJobAgentConfigurationChanged currently
marshals the JobAgents slice directly which is order-sensitive and will signal
changes when the same agents are returned in a different order; fix by
normalizing order before comparison: in isJobAgentConfigurationChanged, sort
both oldDeployment.JobAgents and newDeployment.JobAgents by a stable key (e.g.,
each DeploymentJobAgent.Ref) or build maps keyed by Ref and compare entries
element-by-element, then marshal/compare the normalized representations (or
compare fields directly) so the comparison is order-insensitive; ensure you
reference JobAgentId, JobAgentConfig and JobAgents in the same function and
preserve nil handling.
- Around line 203-206: HandleDeploymentUpdated currently aborts when
getOldDeployment(ws, deployment.Id) returns an error, which drops out-of-order
or replayed update events; instead, detect the "not found" case and fall back to
inserting/upserting the new deployment (ws.Deployments().Upsert(deployment)) or
trigger a full reconciliation for that deployment rather than returning the
error. Modify the error handling around getOldDeployment to only return on
unexpected errors, and for a missing-old-deployment case call
ws.Deployments().Upsert(...) (or invoke the existing reconciliation routine)
before continuing the update processing in HandleDeploymentUpdated.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go`:
- Around line 179-182: Update the test comment to accurately describe the
assertion: replace the misleading comment "No job agent configured and no
jobAgents list — returns empty jobs" with something like "No job agent
configured and no jobAgents list — returns a single InvalidJobAgent sentinel
job" so it matches the subsequent assertions on jobs (variable jobs) and the
expected status oapi.JobStatusInvalidJobAgent.
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Around line 134-144: The loop currently creates jobs via dispatchJobForAgent
and returns nil on first error, leaving previously created jobs in
JobStatusPending orphaned; modify the behavior to avoid partial-state: either
(A) perform an immediate rollback inside the function by deleting any jobs
accumulated in newJobs when dispatchJobForAgent returns an error (call the
appropriate delete method such as e.deleteJob(ctx, job.ID) or
e.store.DeleteJob(ctx, job.ID) for each job), or (B) change the function
signature to return ([]*oapi.Job, error) and return the partial newJobs slice
with the error so the caller can handle cleanup; ensure you reference
dispatchJobForAgent, newJobs and JobStatusPending when implementing the chosen
approach.
---
Outside diff comments:
In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go`:
- Around line 141-156: reconcileTargets currently skips reconciling when a
Deployment uses the new multi-agent field because it only checks
Deployment.JobAgentId; change the guard to run the reconcile body when either
JobAgentId is set/non-empty OR JobAgents is non-nil with length > 0. Update the
same logic in HandleDeploymentCreated so creation-path deployments using
JobAgents are not skipped. Locate reconcileTargets and HandleDeploymentCreated
and replace the single-field check (deployment.JobAgentId != nil &&
*deployment.JobAgentId != "") with a combined condition that checks JobAgentId
OR (deployment.JobAgents != nil && len(*deployment.JobAgents) > 0) before
calling ReleaseManager.DirtyDesiredRelease, RecomputeState and ReconcileTarget.
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Around line 55-76: Add a short Go doc comment above the exported function
InvalidDeploymentAgentsJob that explains its purpose and behavior: that it
constructs and returns an oapi.Job representing an invalid deployment agents
error for a given release and deployment (including the message, Status set to
oapi.JobStatusInvalidJobAgent, timestamps, and metadata initialization), and
mention the parameters (releaseID, deploymentName, action) and their roles; keep
the comment concise and follow godoc style (starting with the function name).
- Around line 173-211: The trace steps "Configure job" and "Create job" are
being recorded with trace.StepResultPass before the actual work
(buildJobAgentConfig and buildDispatchContext) completes; either move the
action.AddStep calls so they run after f.buildJobAgentConfig(...) and
f.buildDispatchContext(...) return successfully, or emit them initially with a
non-passing state (e.g., pending) and update them to StepResultPass only after
those functions succeed; locate the action.AddStep calls, the jobId creation and
the return of &oapi.Job{...}, and ensure action.AddStep invocations reflect the
real outcome of buildJobAgentConfig and buildDispatchContext (or are moved to
after those calls).
---
Duplicate comments:
In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go`:
- Around line 324-327: The retrigger logic skips jobs whose JobAgentId is empty
(added by InvalidDeploymentAgentsJob) because ws.JobAgents().Get(job.JobAgentId)
always fails; change the logic to handle empty JobAgentId by resolving the agent
from the deployment configuration instead: when job.JobAgentId == "" fetch the
deployment via ws.Deployments().Get(job.DeploymentId) and use the deployment's
AgentId (or fallback) to call ws.JobAgents().Get, otherwise use the
job.JobAgentId as before; ensure the rest of the code still continues gracefully
if no agent is found.
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Around line 32-53: Add a Go doc comment for the exported NoAgentConfiguredJob
function: write a short sentence starting with "NoAgentConfiguredJob" describing
that it builds and returns an oapi.Job representing an invalid-job-agent error
for a given release/deployment, and document the parameters (releaseID,
jobAgentID, deploymentName, action) and the returned *oapi.Job; ensure the
comment explains that the function sets Status to oapi.JobStatusInvalidJobAgent,
populates Message and metadata, and notes that it records a trace.Action step
when action is non-nil.
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Around line 59-70: The Upsert call (e.store.Jobs.Upsert) currently ignores
errors and when jobAgentRegistry.Dispatch fails the already-upserted newJob
remains JobStatusPending; change this by checking and handling the error return
from e.store.Jobs.Upsert (return the error after recording it on the span) and,
if dispatch fails, update newJob.Status to a non-pending failure status (e.g.,
JobStatusFailed or an existing dispatch-failure status), persist that change via
e.store.Jobs.Upsert/Update, record the error on the span (span.RecordError /
span.SetStatus) and then return the updated job along with the dispatch error so
the store reflects the failure instead of leaving the job Pending.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go`:
- Around line 158-167: The function getOldDeployment contains a redundant nil
check: after calling ws.Deployments().Get(deploymentID) you already handle the
not-found case with the ok boolean; remove the if oldDeployment == nil branch
and simply return *oldDeployment at the end (keeping the existing error returned
when !ok). This targets getOldDeployment and the oldDeployment variable returned
by ws.Deployments().Get; delete the unnecessary nil guard and rely on the ok
check to signal not-found.
Comment on lines
+169
to
+191
| func isJobAgentConfigurationChanged(oldDeployment *oapi.Deployment, newDeployment *oapi.Deployment) bool { | ||
| oldAgentId := "" | ||
| if oldDeployment.JobAgentId != nil { | ||
| oldAgentId = *oldDeployment.JobAgentId | ||
| } | ||
| newAgentId := "" | ||
| if newDeployment.JobAgentId != nil { | ||
| newAgentId = *newDeployment.JobAgentId | ||
| } | ||
| if oldAgentId != newAgentId { | ||
| return true | ||
| } | ||
|
|
||
| oldConfig, _ := json.Marshal(oldDeployment.JobAgentConfig) | ||
| newConfig, _ := json.Marshal(newDeployment.JobAgentConfig) | ||
| if string(oldConfig) != string(newConfig) { | ||
| return true | ||
| } | ||
|
|
||
| oldAgents, _ := json.Marshal(oldDeployment.JobAgents) | ||
| newAgents, _ := json.Marshal(newDeployment.JobAgents) | ||
| return string(oldAgents) != string(newAgents) | ||
| } |
Contributor
There was a problem hiding this comment.
isJobAgentConfigurationChanged is order-sensitive for the JobAgents slice.
json.Marshal of a []DeploymentJobAgent preserves element order. If the API or the database returns the same agents in a different order, this function returns true and triggers an unnecessary full reconciliation (with skipEligibilityCheck=true) across all release targets.
Consider sorting the slice by a stable key (e.g., agent Ref) before marshaling, or comparing element-by-element after sorting.
🛠️ Proposed fix
+import "sort"
+
func isJobAgentConfigurationChanged(oldDeployment *oapi.Deployment, newDeployment *oapi.Deployment) bool {
// ... (JobAgentId and JobAgentConfig comparison unchanged) ...
+ // Normalize order before comparing to avoid spurious change detection.
+ sortedAgents := func(agents *[]oapi.DeploymentJobAgent) []oapi.DeploymentJobAgent {
+ if agents == nil {
+ return nil
+ }
+ cp := append([]oapi.DeploymentJobAgent{}, *agents...)
+ sort.Slice(cp, func(i, j int) bool { return cp[i].Ref < cp[j].Ref })
+ return cp
+ }
oldAgents, _ := json.Marshal(sortedAgents(oldDeployment.JobAgents))
newAgents, _ := json.Marshal(sortedAgents(newDeployment.JobAgents))
return string(oldAgents) != string(newAgents)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go` around
lines 169 - 191, The function isJobAgentConfigurationChanged currently marshals
the JobAgents slice directly which is order-sensitive and will signal changes
when the same agents are returned in a different order; fix by normalizing order
before comparison: in isJobAgentConfigurationChanged, sort both
oldDeployment.JobAgents and newDeployment.JobAgents by a stable key (e.g., each
DeploymentJobAgent.Ref) or build maps keyed by Ref and compare entries
element-by-element, then marshal/compare the normalized representations (or
compare fields directly) so the comparison is order-insensitive; ensure you
reference JobAgentId, JobAgentConfig and JobAgents in the same function and
preserve nil handling.
Member
Author
There was a problem hiding this comment.
i'm okay with this behavior
Contributor
There was a problem hiding this comment.
@adityachoudhari26, understood! I'll note that the order-sensitive comparison is intentional for your use case.
🧠 Learnings used
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 655
File: apps/workspace-engine/pkg/engine/workspace/fluent.go:166-171
Timestamp: 2025-08-12T20:49:05.086Z
Learning: The UpdateDeploymentVersions() method with OperationCreate case in apps/workspace-engine/pkg/engine/workspace/fluent.go is specifically designed only for creating new deployment versions, not for handling potential duplicates or existing versions.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 696
File: apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go:63-67
Timestamp: 2025-10-24T00:02:29.723Z
Learning: In workspace-engine replay mode (apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go), jobs are created with Pending status even during replay because job updates are still accepted and processed. If a job remains Pending after replay completes, it genuinely is still pending because no updates occurred during replay.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 637
File: packages/events/src/kafka/client.ts:10-16
Timestamp: 2025-08-01T04:41:41.345Z
Learning: User adityachoudhari26 prefers not to add null safety checks for required environment variables when they are guaranteed to be present in their deployment configuration, similar to their preference for simplicity over defensive programming in test code.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
Learnt from: zacharyblasczyk
Repo: ctrlplanedev/ctrlplane PR: 408
File: apps/webservice/src/components/form/job-agent/JobAgentJenkinsPipelineConfig.tsx:26-31
Timestamp: 2025-04-12T22:08:13.790Z
Learning: For Jenkins job configuration, two approaches are needed: (1) a simple URL input form for airgapped environments (current focus) and (2) a dropdown selection interface for non-airgapped environments where the Jenkins server is accessible. A component similar to DeploymentJobAgentGithubConfig.tsx is preferred.
Learnt from: zacharyblasczyk
Repo: ctrlplanedev/ctrlplane PR: 408
File: apps/webservice/src/components/form/job-agent/JobAgentJenkinsPipelineConfig.tsx:26-31
Timestamp: 2025-04-12T22:08:13.790Z
Learning: For Jenkins job configuration, two approaches are needed: (1) a simple URL input form for airgapped environments (current focus) and (2) a dropdown selection interface for non-airgapped environments where the Jenkins server is accessible. A component similar to DeploymentJobAgentGithubConfig.tsx is preferred.
Comment on lines
179
to
182
| // No job agent configured and no jobAgents list — returns empty jobs | ||
| require.NoError(t, err) | ||
| require.NotNil(t, job) | ||
| assert.Equal(t, oapi.JobStatusInvalidJobAgent, job.Status) | ||
| assert.Equal(t, "", job.JobAgentId) | ||
| require.Len(t, jobs, 1) | ||
| require.Equal(t, oapi.JobStatusInvalidJobAgent, jobs[0].Status) |
Contributor
There was a problem hiding this comment.
Comment contradicts the assertion below it.
"returns empty jobs" but the test immediately asserts Len(jobs, 1). It should read something like "returns a single InvalidJobAgent sentinel job".
✏️ Proposed fix
- // No job agent configured and no jobAgents list — returns empty jobs
+ // No job agent configured and no jobAgents list — returns a single InvalidJobAgent sentinel job.
require.NoError(t, err)
require.Len(t, jobs, 1)
require.Equal(t, oapi.JobStatusInvalidJobAgent, jobs[0].Status)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // No job agent configured and no jobAgents list — returns empty jobs | |
| require.NoError(t, err) | |
| require.NotNil(t, job) | |
| assert.Equal(t, oapi.JobStatusInvalidJobAgent, job.Status) | |
| assert.Equal(t, "", job.JobAgentId) | |
| require.Len(t, jobs, 1) | |
| require.Equal(t, oapi.JobStatusInvalidJobAgent, jobs[0].Status) | |
| // No job agent configured and no jobAgents list — returns a single InvalidJobAgent sentinel job. | |
| require.NoError(t, err) | |
| require.Len(t, jobs, 1) | |
| require.Equal(t, oapi.JobStatusInvalidJobAgent, jobs[0].Status) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go`
around lines 179 - 182, Update the test comment to accurately describe the
assertion: replace the misleading comment "No job agent configured and no
jobAgents list — returns empty jobs" with something like "No job agent
configured and no jobAgents list — returns a single InvalidJobAgent sentinel
job" so it matches the subsequent assertions on jobs (variable jobs) and the
expected status oapi.JobStatusInvalidJobAgent.
apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go
Show resolved
Hide resolved
adityachoudhari26
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector_test.go (3)
25-35:makeJobAgent: JSON unmarshal for config is unused noise.The
json.Unmarshalintocfgsets{"type":"custom"}but theConfigfield of the returned agent is never asserted in any test. The helper can be simplified:♻️ Proposed simplification
func makeJobAgent(id, name, agentType string) *oapi.JobAgent { - cfg := oapi.JobAgentConfig{} - _ = json.Unmarshal([]byte(`{"type":"custom"}`), &cfg) return &oapi.JobAgent{ Id: id, WorkspaceId: "test-workspace", Name: name, Type: agentType, - Config: cfg, + Config: oapi.JobAgentConfig{}, } }If a non-empty config is required to satisfy internal validation in
SelectAgents, keep it—but then add a comment explaining why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector_test.go` around lines 25 - 35, The helper makeJobAgent currently performs a json.Unmarshal into cfg (`{"type":"custom"}`) but that populated Config is never asserted in tests and adds noise; simplify by removing the json.Unmarshal and either leave Config as the zero-value cfg or explicitly set a minimal meaningful Config only if SelectAgents validation requires it (in which case add a comment referencing SelectAgents to explain why a non-empty Config is necessary). Update makeJobAgent to return &oapi.JobAgent{..., Config: cfg} without the unused Unmarshal, or keep the Unmarshal and add a clarifying comment if tests rely on a non-empty Config.
320-341:expectCountis redundant in the table struct.
expectCountalways equalslen(expectIDs), making it dead weight. Uselen(tt.expectIDs)directly in therequire.Lencall.♻️ Proposed simplification
tests := []struct { name string ifA string ifB string expectIDs []string - expectCount int }{ { name: "first true second false", ifA: "true", ifB: "false", expectIDs: []string{agentA}, - expectCount: 1, }, { name: "first false second true", ifA: "false", ifB: "true", expectIDs: []string{agentB}, - expectCount: 1, }, } // in loop body: -require.Len(t, agents, tt.expectCount) +require.Len(t, agents, len(tt.expectIDs))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector_test.go` around lines 320 - 341, Remove the redundant expectCount field from the test table and update the assertion to use len(tt.expectIDs) directly; specifically, delete the expectCount entry in the tests slice (the test table struct and each literal) and change the require.Len call that currently uses tt.expectCount to require.Len(..., len(tt.expectIDs)) in the test function (deployment_agent_selector_test.go, the tests variable and its loop where require.Len is called).
476-526: Missing no-match variants for environment name and deployment name CEL tests.
TestSelectAgents_CEL_ResourceMetadataMatchis paired withTestSelectAgents_CEL_ResourceMetadataNoMatch, butTestSelectAgents_CEL_EnvironmentNameMatchandTestSelectAgents_CEL_DeploymentNameMatchhave no complementary no-match tests. For consistency and to confirm the CEL condition actually filters (rather than always matching), add:
TestSelectAgents_CEL_EnvironmentNameNoMatch— environment name"staging"against conditionenvironment.name == "production", expect empty result.TestSelectAgents_CEL_DeploymentNameNoMatch— deployment name"my-deploy"against conditiondeployment.name == "other-deploy", expect empty result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector_test.go` around lines 476 - 526, Add two new tests mirroring the positive CEL tests but asserting no matches: create TestSelectAgents_CEL_EnvironmentNameNoMatch by copying TestSelectAgents_CEL_EnvironmentNameMatch, set the stored environment name to "staging" while keeping the agent If condition environment.name == "production", call NewDeploymentAgentsSelector(...).SelectAgents(), require no error and require.Len(agents, 0); similarly create TestSelectAgents_CEL_DeploymentNameNoMatch by copying TestSelectAgents_CEL_DeploymentNameMatch, set the deployment name to "my-deploy" but use the agent If condition deployment.name == "other-deploy", call SelectAgents() and assert no error and zero agents; reuse makeEnvironment/makeDeployment/makeRelease/makeJobAgent and NewDeploymentAgentsSelector to locate code.apps/workspace-engine/test/e2e/engine_deployment_test.go (2)
1186-1241:AllConditionsFalsetest: release-based filtering may silently miss theInvalidJobAgentjob.The loop uses
Releases().Get(job.ReleaseId)withif !ok { continue }to filter by deployment. If theInvalidJobAgentjob is created before the release is persisted to the store (or if the job carries a nil/emptyReleaseId), the job is silently skipped anddeploymentJobsstays 0 —assert.Equal(t, 1, deploymentJobs, ...)then becomes a false-positive pass. The sibling testTestEngine_AddingAgentToDeploymentRetriggersInvalidJobs(lines 501–509) iteratesJobs().Items()and filters byjob.Statusdirectly, without any release lookup.Consider aligning with that pattern to make the assertion robust:
♻️ Alternative filtering approach
- allJobs := engine.Workspace().Jobs().Items() - - deploymentJobs := 0 - for _, job := range allJobs { - release, ok := engine.Workspace().Releases().Get(job.ReleaseId) - if !ok { - continue - } - if release.ReleaseTarget.DeploymentId == deploymentID { - deploymentJobs++ - assert.Equal(t, oapi.JobStatusInvalidJobAgent, job.Status, - "when no agents match, should create an InvalidJobAgent job") - } - } - - assert.Equal(t, 1, deploymentJobs, "should have 1 job with InvalidJobAgent status") + invalidJobAgentJobs := 0 + for _, job := range engine.Workspace().Jobs().Items() { + if job.Status == oapi.JobStatusInvalidJobAgent { + invalidJobAgentJobs++ + } + } + assert.Equal(t, 1, invalidJobAgentJobs, "should have 1 job with InvalidJobAgent status when all conditions are false")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/e2e/engine_deployment_test.go` around lines 1186 - 1241, The test TestEngine_DeploymentJobAgentsArray_AllConditionsFalse can miss the InvalidJobAgent job due to checking Releases().Get(job.ReleaseId) and skipping jobs with missing/unpersisted ReleaseId; update the job-counting loop to mirror TestEngine_AddingAgentToDeploymentRetriggersInvalidJobs by filtering directly on job.Status == oapi.JobStatusInvalidJobAgent (and optionally also check job.ReleaseId == deploymentID if you want to ensure deployment-scoped assertions) instead of using Releases().Get(job.ReleaseId) so the assertion for one InvalidJobAgent is robust.
367-368:TestEngine_DeploymentJobAgentUpdatestill uses the old in-place mutation pattern — consider updating for consistency.
*d.JobAgentId = jobAgentID2mutates the string in-place through the pointer returned byGet. IfGetreturns an internal pointer, the internal state changes before the event fires, so the post-event assertion (*d.JobAgentId != jobAgentID2) trivially passes regardless of whether the handler actually processed the update. The same concern motivated the shallow-copy changes in this PR.♻️ Proposed fix to apply the shallow-copy pattern consistently
- // Update deployment to use different job agent - *d.JobAgentId = jobAgentID2 - engine.PushEvent(ctx, handler.DeploymentUpdate, d) + // Update deployment to use different job agent + dep := *d + dep.JobAgentId = &jobAgentID2 + engine.PushEvent(ctx, handler.DeploymentUpdate, &dep)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/e2e/engine_deployment_test.go` around lines 367 - 368, TestEngine_DeploymentJobAgentUpdate mutates the deployment via *d.JobAgentId = jobAgentID2 (d is from Get) which can change internal state prematurely; instead make a shallow copy of the deployment struct, set JobAgentId on the copy, and pass that copy to engine.PushEvent so the original d remains unchanged for the post-event assertion. Locate the test function TestEngine_DeploymentJobAgentUpdate, the variable d returned by Get, and replace the in-place mutation of d.JobAgentId with creating a newDeployment := *d (shallow copy), set newDeployment.JobAgentId = &jobAgentID2 (or equivalent), then call engine.PushEvent(ctx, handler.DeploymentUpdate, &newDeployment) and keep the final assertion against *d.JobAgentId to verify the handler applied the update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector_test.go`:
- Around line 25-35: The helper makeJobAgent currently performs a json.Unmarshal
into cfg (`{"type":"custom"}`) but that populated Config is never asserted in
tests and adds noise; simplify by removing the json.Unmarshal and either leave
Config as the zero-value cfg or explicitly set a minimal meaningful Config only
if SelectAgents validation requires it (in which case add a comment referencing
SelectAgents to explain why a non-empty Config is necessary). Update
makeJobAgent to return &oapi.JobAgent{..., Config: cfg} without the unused
Unmarshal, or keep the Unmarshal and add a clarifying comment if tests rely on a
non-empty Config.
- Around line 320-341: Remove the redundant expectCount field from the test
table and update the assertion to use len(tt.expectIDs) directly; specifically,
delete the expectCount entry in the tests slice (the test table struct and each
literal) and change the require.Len call that currently uses tt.expectCount to
require.Len(..., len(tt.expectIDs)) in the test function
(deployment_agent_selector_test.go, the tests variable and its loop where
require.Len is called).
- Around line 476-526: Add two new tests mirroring the positive CEL tests but
asserting no matches: create TestSelectAgents_CEL_EnvironmentNameNoMatch by
copying TestSelectAgents_CEL_EnvironmentNameMatch, set the stored environment
name to "staging" while keeping the agent If condition environment.name ==
"production", call NewDeploymentAgentsSelector(...).SelectAgents(), require no
error and require.Len(agents, 0); similarly create
TestSelectAgents_CEL_DeploymentNameNoMatch by copying
TestSelectAgents_CEL_DeploymentNameMatch, set the deployment name to "my-deploy"
but use the agent If condition deployment.name == "other-deploy", call
SelectAgents() and assert no error and zero agents; reuse
makeEnvironment/makeDeployment/makeRelease/makeJobAgent and
NewDeploymentAgentsSelector to locate code.
In `@apps/workspace-engine/test/e2e/engine_deployment_test.go`:
- Around line 1186-1241: The test
TestEngine_DeploymentJobAgentsArray_AllConditionsFalse can miss the
InvalidJobAgent job due to checking Releases().Get(job.ReleaseId) and skipping
jobs with missing/unpersisted ReleaseId; update the job-counting loop to mirror
TestEngine_AddingAgentToDeploymentRetriggersInvalidJobs by filtering directly on
job.Status == oapi.JobStatusInvalidJobAgent (and optionally also check
job.ReleaseId == deploymentID if you want to ensure deployment-scoped
assertions) instead of using Releases().Get(job.ReleaseId) so the assertion for
one InvalidJobAgent is robust.
- Around line 367-368: TestEngine_DeploymentJobAgentUpdate mutates the
deployment via *d.JobAgentId = jobAgentID2 (d is from Get) which can change
internal state prematurely; instead make a shallow copy of the deployment
struct, set JobAgentId on the copy, and pass that copy to engine.PushEvent so
the original d remains unchanged for the post-event assertion. Locate the test
function TestEngine_DeploymentJobAgentUpdate, the variable d returned by Get,
and replace the in-place mutation of d.JobAgentId with creating a newDeployment
:= *d (shallow copy), set newDeployment.JobAgentId = &jobAgentID2 (or
equivalent), then call engine.PushEvent(ctx, handler.DeploymentUpdate,
&newDeployment) and keep the final assertion against *d.JobAgentId to verify the
handler applied the update.
…multiple-job-agents
jsbroks
reviewed
Feb 18, 2026
| properties: { | ||
| ref: { type: 'string' }, | ||
| config: openapi.schemaRef('JobAgentConfig'), | ||
| 'if': { type: 'string', description: 'CEL expression to determine if the job agent should be used' }, |
Member
There was a problem hiding this comment.
on the fence about if we should call this if, or selector. Selector matchs our pattern, and we can always wrap it as if in the frontend clients
…multiple-job-agents
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/oapi/openapi.json (1)
179-207:⚠️ Potential issue | 🟡 MinorDeploymentJobAgent.if should not be required in the schema.
The
Deployment.jobAgentConfigfield correctly remains required—the backend always populates it (as empty object if nil). However, there is a schema mismatch withDeploymentJobAgent: line 256 marks theiffield as required, but the code atdeployment_agent_selector.go:87checksif deploymentJobAgent.If != "", indicating it can be omitted or empty. The schema should either makeifoptional or provide a default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/oapi/openapi.json` around lines 179 - 207, The schema marks the DeploymentJobAgent property named "if" as required but the runtime (DeploymentJobAgent.If checked in deployment_agent_selector.go) treats it as optional/emptyable; update the components/schemas/DeploymentJobAgent entry in openapi.json to either remove "if" from the required array or add a default empty string for the "if" property so the schema matches runtime behavior (i.e., make "if" optional by deleting it from the required list or set "default": "" on the "if" property).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/oapi/openapi.json`:
- Around line 240-258: Update the OpenAPI schema for the DeploymentJobAgent
object: remove "if" from the required list so the CEL expression is optional,
add a "default" (empty string) to the "if" property to reflect that omission is
treated as an empty expression by deployment_agent_selector.go, and add a
descriptive "description" to the "ref" property explaining what it references
(e.g., the identifier/name of the job agent implementation or registry entry).
Ensure the schema still references JobAgentConfig for "config" and keep types
unchanged for compatibility with existing code.
---
Outside diff comments:
In `@apps/workspace-engine/oapi/openapi.json`:
- Around line 179-207: The schema marks the DeploymentJobAgent property named
"if" as required but the runtime (DeploymentJobAgent.If checked in
deployment_agent_selector.go) treats it as optional/emptyable; update the
components/schemas/DeploymentJobAgent entry in openapi.json to either remove
"if" from the required array or add a default empty string for the "if" property
so the schema matches runtime behavior (i.e., make "if" optional by deleting it
from the required list or set "default": "" on the "if" property).
Comment on lines
240
to
258
| "DeploymentJobAgent": { | ||
| "properties": { | ||
| "config": { | ||
| "$ref": "#/components/schemas/JobAgentConfig" | ||
| }, | ||
| "if": { | ||
| "description": "CEL expression to determine if the job agent should be used", | ||
| "type": "string" | ||
| }, | ||
| "ref": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "ref", | ||
| "config", | ||
| "if" | ||
| ], | ||
| "type": "object" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'DeploymentJobAgent|jobAgents|DeploymentAgentsSelector|JobAgentsSelector' -g '*.{go,ts,tsx,jsonnet}' --max-count=50Repository: ctrlplanedev/ctrlplane
Length of output: 48932
🏁 Script executed:
# First, examine the deployment agent selector implementation
head -150 apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.goRepository: ctrlplanedev/ctrlplane
Length of output: 3431
🏁 Script executed:
# Check the jsonnet schema definition
cat apps/workspace-engine/oapi/spec/schemas/deployments.jsonnetRepository: ctrlplanedev/ctrlplane
Length of output: 3342
🏁 Script executed:
# Check test patterns for empty/missing 'if'
grep -A 5 -B 5 "If: \"\"" apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector_test.go | head -40Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Look at specific test cases to understand behavior
sed -n '185,290p' apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector_test.goRepository: ctrlplanedev/ctrlplane
Length of output: 3631
🏁 Script executed:
# Check the generated DeploymentJobAgent type
sed -n '315,330p' apps/workspace-engine/pkg/oapi/oapi.gen.goRepository: ctrlplanedev/ctrlplane
Length of output: 620
🏁 Script executed:
# Verify the key logic: confirm how empty If is handled and what ref points to
grep -n "deploymentJobAgent.If\|deploymentJobAgent.Ref" apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.goRepository: ctrlplanedev/ctrlplane
Length of output: 437
🏁 Script executed:
# Check if the file is using Prettier formatting
head -20 apps/workspace-engine/oapi/openapi.jsonRepository: ctrlplanedev/ctrlplane
Length of output: 653
Make DeploymentJobAgent.if optional and document ref.
Line 87 of deployment_agent_selector.go shows that empty if expressions are treated as valid (the agent is included). The current schema requires if, but JSON clients that omit it will default to an empty string—which works fine in the implementation. Similarly, ref has no description. Align the schema with actual behavior by removing if from the required array, adding a default value, and documenting what ref references.
🧭 Suggested schema tweaks
"if": {
"description": "CEL expression to determine if the job agent should be used",
- "type": "string"
+ "type": "string",
+ "default": "true"
},
"ref": {
- "type": "string"
+ "description": "Reference to the job agent (JobAgent.id)",
+ "type": "string"
}
...
"required": [
"ref",
- "config",
- "if"
+ "config"
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "DeploymentJobAgent": { | |
| "properties": { | |
| "config": { | |
| "$ref": "#/components/schemas/JobAgentConfig" | |
| }, | |
| "if": { | |
| "description": "CEL expression to determine if the job agent should be used", | |
| "type": "string" | |
| }, | |
| "ref": { | |
| "type": "string" | |
| } | |
| }, | |
| "required": [ | |
| "ref", | |
| "config", | |
| "if" | |
| ], | |
| "type": "object" | |
| "DeploymentJobAgent": { | |
| "properties": { | |
| "config": { | |
| "$ref": "#/components/schemas/JobAgentConfig" | |
| }, | |
| "if": { | |
| "description": "CEL expression to determine if the job agent should be used", | |
| "type": "string", | |
| "default": "true" | |
| }, | |
| "ref": { | |
| "description": "Reference to the job agent (JobAgent.id)", | |
| "type": "string" | |
| } | |
| }, | |
| "required": [ | |
| "ref", | |
| "config" | |
| ], | |
| "type": "object" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/oapi/openapi.json` around lines 240 - 258, Update the
OpenAPI schema for the DeploymentJobAgent object: remove "if" from the required
list so the CEL expression is optional, add a "default" (empty string) to the
"if" property to reflect that omission is treated as an empty expression by
deployment_agent_selector.go, and add a descriptive "description" to the "ref"
property explaining what it references (e.g., the identifier/name of the job
agent implementation or registry entry). Ensure the schema still references
JobAgentConfig for "config" and keep types unchanged for compatibility with
existing code.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/events/handler/deployment/deployment.go (1)
268-271:⚠️ Potential issue | 🟠 MajorRemove unused
jobWithReleaseTargettype and related dead code.The type
jobWithReleaseTarget(lines 268–271), along withgetAllJobsWithReleaseTargetandgetJobsToRetrigger, is unused.getJobsToRetriggeris never called, making the entire chain of dependent code unreachable. Remove these functions and type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go` around lines 268 - 271, Remove the unused type and functions: delete the jobWithReleaseTarget struct and the functions getAllJobsWithReleaseTarget and getJobsToRetrigger (and any helper functions they call) from deployment.go; ensure you also remove any dead imports or references that become unused after removing these symbols and run a build to confirm there are no remaining references to jobWithReleaseTarget, getAllJobsWithReleaseTarget, or getJobsToRetrigger.
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.go (2)
17-23: Add doc comment for exported constructor.
NewDeploymentAgentsSelectoris exported but lacks documentation explaining its purpose and parameters.✏️ Suggested doc comment
+// NewDeploymentAgentsSelector creates a selector that determines which job agents +// should be used for a given deployment and release, supporting both legacy single-agent +// and new multi-agent configurations with CEL-based conditional selection. func NewDeploymentAgentsSelector(store *store.Store, deployment *oapi.Deployment, release *oapi.Release) *DeploymentAgentsSelector {As per coding guidelines, "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.go` around lines 17 - 23, Add a Go doc comment for the exported constructor NewDeploymentAgentsSelector that briefly explains its purpose (to create and return a DeploymentAgentsSelector for selecting job agents for a given deployment release), documents the parameters (store *store.Store, deployment *oapi.Deployment, release *oapi.Release) and describes the returned *DeploymentAgentsSelector; place the comment immediately above the NewDeploymentAgentsSelector function and mention the relationship to the DeploymentAgentsSelector type so godoc and linters recognize it as package-level documentation.
66-110: Add doc comment for exported methodSelectAgents.This method implements the core agent selection logic with legacy fallback, CEL evaluation, and multi-agent support, but lacks documentation.
✏️ Suggested doc comment
+// SelectAgents returns the job agents configured for this deployment. It supports: +// - Legacy mode: returns single agent when deployment.JobAgentId is set +// - Multi-agent mode: evaluates each deployment.JobAgents entry's If expression (CEL) +// against the release context and returns matching agents func (s *DeploymentAgentsSelector) SelectAgents() ([]*oapi.JobAgent, error) {As per coding guidelines, "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.go` around lines 66 - 110, Add a Go doc comment for the exported method SelectAgents on DeploymentAgentsSelector that succinctly describes its purpose (selects JobAgents for a deployment), explains key behaviors (legacy fallback when JobAgentId is set, iteration over deployment.JobAgents, CEL compilation/evaluation via jobAgentIfEnv with buildCelContext(), and lookup via s.store.JobAgents.Get), and documents return semantics (returns selected []*oapi.JobAgent on success, empty slice when none, and possible error cases such as CEL build/compile/eval failures or missing agent references). Mention any non-obvious details like the 12-hour CEL env cache and that expressions in deploymentJobAgent.If are optional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Line 70: The call to e.store.Jobs.Upsert(ctx, newJob) ignores the returned
error; modify the code in the function containing that call to capture the error
(err := e.store.Jobs.Upsert(...)), handle it appropriately by logging the error
via the existing logger (including context like newJob.ID) and returning the
error (or failing the surrounding operation) so persistence failures are not
silently ignored; ensure you reference e.store.Jobs.Upsert and newJob when
adding the check and propagate the error out of the enclosing function (or
implement a retry/backoff if the surrounding logic requires it).
---
Outside diff comments:
In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go`:
- Around line 268-271: Remove the unused type and functions: delete the
jobWithReleaseTarget struct and the functions getAllJobsWithReleaseTarget and
getJobsToRetrigger (and any helper functions they call) from deployment.go;
ensure you also remove any dead imports or references that become unused after
removing these symbols and run a build to confirm there are no remaining
references to jobWithReleaseTarget, getAllJobsWithReleaseTarget, or
getJobsToRetrigger.
---
Duplicate comments:
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Around line 145-154: The loop in executor.go appends newJob to newJobs even
when dispatchJobForAgent returns (nil, err), causing nil entries; update the
loop inside the Deploy/dispatch caller so that after calling
e.dispatchJobForAgent(ctx, releaseToDeploy, agent) you record the error
(span.RecordError/SetStatus) and continue the loop without appending when newJob
== nil (i.e., only append to newJobs when newJob != nil), referencing the
newJobs slice and the dispatchJobForAgent call to locate the change.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.go`:
- Around line 17-23: Add a Go doc comment for the exported constructor
NewDeploymentAgentsSelector that briefly explains its purpose (to create and
return a DeploymentAgentsSelector for selecting job agents for a given
deployment release), documents the parameters (store *store.Store, deployment
*oapi.Deployment, release *oapi.Release) and describes the returned
*DeploymentAgentsSelector; place the comment immediately above the
NewDeploymentAgentsSelector function and mention the relationship to the
DeploymentAgentsSelector type so godoc and linters recognize it as package-level
documentation.
- Around line 66-110: Add a Go doc comment for the exported method SelectAgents
on DeploymentAgentsSelector that succinctly describes its purpose (selects
JobAgents for a deployment), explains key behaviors (legacy fallback when
JobAgentId is set, iteration over deployment.JobAgents, CEL
compilation/evaluation via jobAgentIfEnv with buildCelContext(), and lookup via
s.store.JobAgents.Get), and documents return semantics (returns selected
[]*oapi.JobAgent on success, empty slice when none, and possible error cases
such as CEL build/compile/eval failures or missing agent references). Mention
any non-obvious details like the 12-hour CEL env cache and that expressions in
deploymentJobAgent.If are optional.
| return nil, err | ||
| } | ||
|
|
||
| e.store.Jobs.Upsert(ctx, newJob) |
Contributor
There was a problem hiding this comment.
Upsert error is not checked.
The job persistence error from Upsert is silently ignored. If persistence fails, the job may not be recoverable.
🛠️ Proposed fix
- e.store.Jobs.Upsert(ctx, newJob)
+ if err := e.store.Jobs.Upsert(ctx, newJob); err != nil {
+ span.RecordError(err)
+ span.SetStatus(codes.Error, "failed to persist job")
+ return nil, fmt.Errorf("failed to persist job: %w", err)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go` at
line 70, The call to e.store.Jobs.Upsert(ctx, newJob) ignores the returned
error; modify the code in the function containing that call to capture the error
(err := e.store.Jobs.Upsert(...)), handle it appropriately by logging the error
via the existing logger (including context like newJob.ID) and returning the
error (or failing the surrounding operation) so persistence failures are not
silently ignored; ensure you reference e.store.Jobs.Upsert and newJob when
adding the check and propagate the error out of the enclosing function (or
implement a retry/backoff if the surrounding logic requires it).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/workspace-engine/test/e2e/engine_deployment_test.go (1)
614-616:⚠️ Potential issue | 🟡 MinorStale pointer in second update sends
JobAgentId = nilinstead of a name-only changeAfter the
deppush (lines 596–598),dis still the original pointer retrieved at line 591 —d.JobAgentIdisnil. Mutatingd.Nameand pushingdsends aDeploymentUpdateevent where the job agent is absent, not merely an event where only the name changed. The engine may interpret this as "job agent removed" rather than a benign name update, testing a different invariant than the one described.🐛 Proposed fix — re-use `dep` for the name-only update
- // Update random field on the deployment - d.Name = "deployment-2" - engine.PushEvent(ctx, handler.DeploymentUpdate, d) + // Update random field on the deployment (preserve the agent that was set) + dep2 := dep + dep2.Name = "deployment-2" + engine.PushEvent(ctx, handler.DeploymentUpdate, &dep2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/e2e/engine_deployment_test.go` around lines 614 - 616, The second update is mutating the stale pointer d (retrieved earlier) which has dep.JobAgentId == nil, so calling engine.PushEvent(ctx, handler.DeploymentUpdate, d) inadvertently signals removal of the job agent; instead reuse the fresh variable dep (the deployment returned/used in the prior push) when making the name-only change so dep.JobAgentId is preserved — i.e., set dep.Name = "deployment-2" and call engine.PushEvent(ctx, handler.DeploymentUpdate, dep) to ensure only the name is updated.apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go (1)
114-131:⚠️ Potential issue | 🟠 MajorDon’t swallow jobAgents marshal failures.
If JSON encoding fails, the code silently persists[]and drops the agent config. Since this function already returns an error, bubble it up so invalid payloads don’t get lost.🛠️ Suggested fix
jobAgentsJSON := []byte("[]") if d.JobAgents != nil && len(*d.JobAgents) > 0 { - if b, err := json.Marshal(d.JobAgents); err == nil { - jobAgentsJSON = b - } + b, err := json.Marshal(d.JobAgents) + if err != nil { + return db.UpsertDeploymentParams{}, fmt.Errorf("marshal jobAgents: %w", err) + } + jobAgentsJSON = b }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go` around lines 114 - 131, The code currently marshals d.JobAgents into jobAgentsJSON and silently ignores json.Marshal errors, persisting an empty array; update the logic in mapper.go (around jobAgentsJSON, d.JobAgents and the return of db.UpsertDeploymentParams) to check the error from json.Marshal and return it (propagate the error from the mapper function) instead of swallowing it, so that on success JobAgents is set to the produced []byte and on failure the function returns the marshal error to the caller.apps/workspace-engine/pkg/events/handler/deployment/deployment.go (1)
75-85:⚠️ Potential issue | 🟠 MajorReconciliation is gated only on
JobAgentId, notJobAgents.If a deployment uses the new
JobAgentslist without a legacyJobAgentId, the handler skips reconciliation of release targets entirely. This means no initial reconciliation is triggered for multi-agent deployments on creation. Extend the guard to check for both.Suggested fix
- if deployment.JobAgentId != nil && *deployment.JobAgentId != "" { + hasLegacyAgent := deployment.JobAgentId != nil && *deployment.JobAgentId != "" + hasAgentList := deployment.JobAgents != nil && len(*deployment.JobAgents) > 0 + if hasLegacyAgent || hasAgentList { reconileReleaseTargets = append(reconileReleaseTargets, releaseTarget) }Same pattern applies at line 143 in
reconcileTargets().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go` around lines 75 - 85, The reconciliation gating only checks deployment.JobAgentId which skips reconciling release targets for deployments that use the new JobAgents list; update the condition inside the loop that builds reconileReleaseTargets (where releaseTargets are Upserted) to append when either deployment.JobAgentId is non-nil/non-empty OR deployment.JobAgents has elements (e.g., len(deployment.JobAgents) > 0). Also apply the same change in the reconcileTargets() function where the same guard pattern appears so multi-agent deployments trigger initial reconciliation.apps/workspace-engine/pkg/workspace/jobs/factory.go (1)
183-201:⚠️ Potential issue | 🟡 MinorTrace "Create job" step recorded as success before job-building calls that can fail.
action.AddStep("Create job", StepResultPass, "Job created successfully …")is emitted at line 183, butbuildJobAgentConfig(line 193) andbuildDispatchContext(line 198) are called afterwards and can both return errors. If either fails, the trace already contains a spurious success entry while the function propagates an error to the caller.🛠️ Proposed fix – move trace step after successful job construction
- jobId := uuid.New().String() - - if action != nil { - action.AddStep("Create job", trace.StepResultPass, - fmt.Sprintf("Job created successfully with ID %s for release %s", jobId, release.ID())). - AddMetadata("job_id", jobId). - AddMetadata("job_status", string(oapi.JobStatusPending)). - AddMetadata("job_agent_id", jobAgent.Id). - AddMetadata("release_id", release.ID()). - AddMetadata("version_tag", release.Version.Tag) - } - - mergedConfig, err := f.buildJobAgentConfig(release, deployment, jobAgent) + mergedConfig, err := f.buildJobAgentConfig(release, deployment, jobAgent) if err != nil { return nil, fmt.Errorf("failed to get merged job agent config: %w", err) } dispatchContext, err := f.buildDispatchContext(release, deployment, jobAgent, mergedConfig) if err != nil { return nil, fmt.Errorf("failed to build dispatch context: %w", err) } + jobId := uuid.New().String() + + if action != nil { + action.AddStep("Create job", trace.StepResultPass, + fmt.Sprintf("Job created successfully with ID %s for release %s", jobId, release.ID())). + AddMetadata("job_id", jobId). + AddMetadata("job_status", string(oapi.JobStatusPending)). + AddMetadata("job_agent_id", jobAgent.Id). + AddMetadata("release_id", release.ID()). + AddMetadata("version_tag", release.Version.Tag) + } + return &oapi.Job{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobs/factory.go` around lines 183 - 201, The "Create job" trace step is recorded as a success before downstream calls that can fail; move the action.AddStep("Create job", trace.StepResultPass, ...) block so it runs only after buildJobAgentConfig(...) and buildDispatchContext(...) both succeed (i.e., after mergedConfig and dispatchContext are obtained), and ensure any errors from buildJobAgentConfig or buildDispatchContext are recorded as a failing trace step (e.g., StepResultFail) with appropriate metadata before returning; reference action.AddStep, buildJobAgentConfig, and buildDispatchContext to locate and update the code.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/jobs/factory.go (1)
264-292:ctxis unused inCreateJobForWorkflowJob; consider adding an OTel span for consistency.Unlike
CreateJobForRelease, which starts a tracer span withctxat line 142,CreateJobForWorkflowJobdeclaresctx context.Contextbut never uses it. This leaves workflow-job creation invisible in distributed traces.♻️ Proposed refactor – add a span consistent with CreateJobForRelease
func (f *Factory) CreateJobForWorkflowJob(ctx context.Context, wfJob *oapi.WorkflowJob) (*oapi.Job, error) { + _, span := tracer.Start(ctx, "CreateJobForWorkflowJob", + oteltrace.WithAttributes( + attribute.String("workflow_job.id", wfJob.Id), + attribute.String("workflow_run.id", wfJob.WorkflowRunId), + )) + defer span.End() + jobAgent, exists := f.store.JobAgents.Get(wfJob.Ref)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobs/factory.go` around lines 264 - 292, CreateJobForWorkflowJob currently accepts ctx but never uses it; add an OpenTelemetry span like in CreateJobForRelease to ensure tracing coverage. At the start of CreateJobForWorkflowJob, call tracer.Start(ctx, "CreateJobForWorkflowJob") to obtain a new ctx and span, defer span.End(), and use the new ctx for any downstream calls (e.g., buildWorkflowJobConfig and buildWorkflowJobDispatchContext) so those functions receive the traced context; also record any errors on the span before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet`:
- Around line 20-28: The schema marks DeploymentJobAgent.selector as required
which forces the generated Go type to be a non-pointer string; remove "selector"
from the required array in the DeploymentJobAgent JSONNet so selector is
optional (i.e., not required) and the generated Go model becomes *string (or
omitempty), allowing callers to omit it and letting the existing logic in
pkg/workspace/jobagents/deployment_agent_selector.go treat nil/empty as "always
match"; after the change regenerate the OpenAPI/Go client types so the models
reflect the optional selector.
---
Outside diff comments:
In `@apps/workspace-engine/pkg/events/handler/deployment/deployment.go`:
- Around line 75-85: The reconciliation gating only checks deployment.JobAgentId
which skips reconciling release targets for deployments that use the new
JobAgents list; update the condition inside the loop that builds
reconileReleaseTargets (where releaseTargets are Upserted) to append when either
deployment.JobAgentId is non-nil/non-empty OR deployment.JobAgents has elements
(e.g., len(deployment.JobAgents) > 0). Also apply the same change in the
reconcileTargets() function where the same guard pattern appears so multi-agent
deployments trigger initial reconciliation.
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Around line 183-201: The "Create job" trace step is recorded as a success
before downstream calls that can fail; move the action.AddStep("Create job",
trace.StepResultPass, ...) block so it runs only after buildJobAgentConfig(...)
and buildDispatchContext(...) both succeed (i.e., after mergedConfig and
dispatchContext are obtained), and ensure any errors from buildJobAgentConfig or
buildDispatchContext are recorded as a failing trace step (e.g., StepResultFail)
with appropriate metadata before returning; reference action.AddStep,
buildJobAgentConfig, and buildDispatchContext to locate and update the code.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go`:
- Around line 114-131: The code currently marshals d.JobAgents into
jobAgentsJSON and silently ignores json.Marshal errors, persisting an empty
array; update the logic in mapper.go (around jobAgentsJSON, d.JobAgents and the
return of db.UpsertDeploymentParams) to check the error from json.Marshal and
return it (propagate the error from the mapper function) instead of swallowing
it, so that on success JobAgents is set to the produced []byte and on failure
the function returns the marshal error to the caller.
In `@apps/workspace-engine/test/e2e/engine_deployment_test.go`:
- Around line 614-616: The second update is mutating the stale pointer d
(retrieved earlier) which has dep.JobAgentId == nil, so calling
engine.PushEvent(ctx, handler.DeploymentUpdate, d) inadvertently signals removal
of the job agent; instead reuse the fresh variable dep (the deployment
returned/used in the prior push) when making the name-only change so
dep.JobAgentId is preserved — i.e., set dep.Name = "deployment-2" and call
engine.PushEvent(ctx, handler.DeploymentUpdate, dep) to ensure only the name is
updated.
---
Duplicate comments:
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Around line 139-141: The earlier lint/review flag about a plural vs singular
name is resolved for CreateJobForRelease, so remove the stale/duplicate review
marker and any leftover references to the old CreateJobsForRelease check; ensure
the function comment matches the singular function name CreateJobForRelease and
delete the redundant "[duplicate_comment]" note from the file or PR comment
thread.
- Around line 32-33: The doc comment for NoAgentConfiguredJob has been added
already; no code change required in Factory.NoAgentConfiguredJob—remove the
redundant "[duplicate_comment]" marker from the review thread (or dismiss the
duplicate review) so the PR doesn't show a duplicate concern and leave the new
comment as the canonical documentation for NoAgentConfiguredJob.
- Around line 181-201: The previous bug about a shared jobId has been resolved
by generating jobId just once here; ensure that this jobId is the one assigned
to the returned oapi.Job.Id and is the same value added to the trace metadata
(reference symbol: jobId) and that no other code path (e.g., any earlier loops
or calls to buildJobForAgent) generates or overrides a job ID; also remove the
stale/duplicate review marker/comment ([duplicate_comment]) if present so the
code/review no longer implies an unresolved issue; verify buildJobAgentConfig
and buildDispatchContext still accept and use the mergedConfig/dispatchContext
without reintroducing multi-job semantics.
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Around line 63-71: The code currently appends newJob to the results even when
CreateJobForRelease fails; change dispatchJobForAgent (and the similar block
around the other CreateJobForRelease call) so you only append newJob and call
e.store.Jobs.Upsert after a successful return from
e.jobFactory.CreateJobForRelease: check the error from
e.jobFactory.CreateJobForRelease (in dispatchJobForAgent and the other
occurrence), return the partial slice with the error or skip appending when err
!= nil, and ensure Upsert is only invoked when newJob is non-nil and created
successfully.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Around line 264-292: CreateJobForWorkflowJob currently accepts ctx but never
uses it; add an OpenTelemetry span like in CreateJobForRelease to ensure tracing
coverage. At the start of CreateJobForWorkflowJob, call tracer.Start(ctx,
"CreateJobForWorkflowJob") to obtain a new ctx and span, defer span.End(), and
use the new ctx for any downstream calls (e.g., buildWorkflowJobConfig and
buildWorkflowJobDispatchContext) so those functions receive the traced context;
also record any errors on the span before returning.
Comment on lines
+20
to
+28
| DeploymentJobAgent: { | ||
| type: 'object', | ||
| required: ['ref', 'config', 'selector'], | ||
| properties: { | ||
| ref: { type: 'string' }, | ||
| config: openapi.schemaRef('JobAgentConfig'), | ||
| selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used' }, | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the current state of deployments.jsonnet schema
cd apps/workspace-engine && cat -n oapi/spec/schemas/deployments.jsonnet | sed -n '1,50p'Repository: ctrlplanedev/ctrlplane
Length of output: 2007
🏁 Script executed:
# Look for the DeploymentJobAgent definition and surrounding context
cd apps/workspace-engine && rg -n "DeploymentJobAgent" oapi/spec/schemas/deployments.jsonnet -A 10 -B 2Repository: ctrlplanedev/ctrlplane
Length of output: 802
🏁 Script executed:
# Check the generated Go type for DeploymentJobAgent
cd apps/workspace-engine && rg -n "type DeploymentJobAgent struct" pkg/oapi/oapi.gen.go -A 15Repository: ctrlplanedev/ctrlplane
Length of output: 694
🏁 Script executed:
# Find the test mentioned in the review
cd apps/workspace-engine && rg -n "AllAgentsNoCondition" . --type go -B 3 -A 15Repository: ctrlplanedev/ctrlplane
Length of output: 1414
🏁 Script executed:
# Check if there are other uses of DeploymentJobAgent in tests
cd apps/workspace-engine && rg -n "DeploymentJobAgent{" . --type go -B 2 -A 5Repository: ctrlplanedev/ctrlplane
Length of output: 167
🏁 Script executed:
# Search for DeploymentJobAgent struct construction
cd apps/workspace-engine && rg -n "DeploymentJobAgent" test/e2e/engine_deployment_test.go -A 5 -B 2Repository: ctrlplanedev/ctrlplane
Length of output: 7519
🏁 Script executed:
# Check the specific test to see how it uses DeploymentJobAgent
cd apps/workspace-engine && sed -n '1013,1100p' test/e2e/engine_deployment_test.goRepository: ctrlplanedev/ctrlplane
Length of output: 3056
🏁 Script executed:
# Search for where Selector is being set (or not set) in deployments
cd apps/workspace-engine && rg -n "Selector" test/e2e/engine_deployment_test.go | grep -i "agent\|deployment"Repository: ctrlplanedev/ctrlplane
Length of output: 1989
🏁 Script executed:
# Look for all test cases that construct DeploymentJobAgent
cd apps/workspace-engine && rg -n "jobAgents" test/e2e/engine_deployment_test.go -B 5 -A 10Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check how selector is evaluated in the deployment job agent logic
cd apps/workspace-engine && rg -n "selector" pkg/ --type go -i | grep -i "agent\|deployment" | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 3337
🏁 Script executed:
# Look for where DeploymentJobAgent.Selector is actually evaluated/used
cd apps/workspace-engine && rg -n "\.Selector" pkg/ --type go -B 3 -A 3 | grep -i agent -A 5 -B 5Repository: ctrlplanedev/ctrlplane
Length of output: 5529
🏁 Script executed:
# Get the full context of the deployment_agent_selector.go logic
cd apps/workspace-engine && sed -n '85,105p' pkg/workspace/jobagents/deployment_agent_selector.goRepository: ctrlplanedev/ctrlplane
Length of output: 866
🏁 Script executed:
# Check if there are other places where DeploymentJobAgent.Selector is evaluated
cd apps/workspace-engine && rg -n "DeploymentJobAgent" pkg/workspace/jobagents/ --type go -A 8 | grep -i "selector"Repository: ctrlplanedev/ctrlplane
Length of output: 17722
selector should be optional, not required
The tests (e.g., TestEngine_DeploymentJobAgentsArray_AllAgentsNoCondition) construct DeploymentJobAgent without a Selector field, relying on the Go zero value "". With selector marked required in the schema, the generated type is Selector string (non-pointer), making it impossible to distinguish "no filter" from "empty string" at the API boundary. The application logic in pkg/workspace/jobagents/deployment_agent_selector.go (lines 90–100) treats empty string as "always match"—it skips CEL compilation when Selector != "" is false—but an empty string is not a valid CEL expression.
Making selector optional cleanly represents "omit or leave empty to always use this agent":
♻️ Proposed fix
DeploymentJobAgent: {
type: 'object',
- required: ['ref', 'config', 'selector'],
+ required: ['ref', 'config'],
properties: {
ref: { type: 'string', description: 'Job agent ID to use for this configuration' },
config: openapi.schemaRef('JobAgentConfig'),
- selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used' },
+ selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used. Omit or leave empty to always use this agent.' },
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DeploymentJobAgent: { | |
| type: 'object', | |
| required: ['ref', 'config', 'selector'], | |
| properties: { | |
| ref: { type: 'string' }, | |
| config: openapi.schemaRef('JobAgentConfig'), | |
| selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used' }, | |
| }, | |
| }, | |
| DeploymentJobAgent: { | |
| type: 'object', | |
| required: ['ref', 'config'], | |
| properties: { | |
| ref: { type: 'string', description: 'Job agent ID to use for this configuration' }, | |
| config: openapi.schemaRef('JobAgentConfig'), | |
| selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used. Omit or leave empty to always use this agent.' }, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet` around lines 20
- 28, The schema marks DeploymentJobAgent.selector as required which forces the
generated Go type to be a non-pointer string; remove "selector" from the
required array in the DeploymentJobAgent JSONNet so selector is optional (i.e.,
not required) and the generated Go model becomes *string (or omitempty),
allowing callers to omit it and letting the existing logic in
pkg/workspace/jobagents/deployment_agent_selector.go treat nil/empty as "always
match"; after the change regenerate the OpenAPI/Go client types so the models
reflect the optional selector.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior Changes
Chores
Tests